Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow registering global handlers for GAP_TRY/GAP_CATCH #4080

Merged
merged 1 commit into from
Jul 23, 2020
Merged

Allow registering global handlers for GAP_TRY/GAP_CATCH #4080

merged 1 commit into from
Jul 23, 2020

Conversation

rbehrends
Copy link
Contributor

This change allows packages to register global handlers in order to be notified when a GAP_TRY statement is started, when it succeeds, or when it results in an error.

The main motivation here is the GAP-Julia integration, where this change would allow us to handle GAP errors without creating unnecessary overhead by wrapping lots of short calls into GAP in GAP_TRY/GAP_CATCH sections.

Such a feature might also be useful in other situations.

@rbehrends rbehrends added kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements topic: kernel topic: error handling topic: julia Julia GC integration and related matters labels Jul 21, 2020
@rbehrends rbehrends requested a review from fingolfin July 21, 2020 12:31
@coveralls
Copy link

coveralls commented Jul 21, 2020 •
edited
Loading

Coverage Status

Coverage increased (+0.001%) to 85.749% when pulling 685b4e2 on rbehrends:try-catch-handlers into 9fe4735 on gap-system:master.

fingolfin
fingolfin reviewed Jul 21, 2020
View reviewed changes
Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems sensible enough to me. Of course a a test, e.g. in tst/testlibgap, would be super nice... In the meantime, at least a comment somewhere stating that this is (or rather: will be) used by the GAP-Julia interface would be appropriate. Else this might get remove again for being unused...

src/trycatch.h Outdated
** The function RegisterTryCatchObserver() allows the installation of
** global exception handlers that are being called whenever GAP_TRY
** or CALL_WITH_CATCH() code is executed. The mode parameter signals
** whether it is has been called at the beginning of the section,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
** whether it is has been called at the beginning of the section,
** whether it has been called at the beginning of the section,

src/sysjmp.c Outdated Show resolved Hide resolved
src/sysjmp.c Outdated Show resolved Hide resolved
src/sysjmp.c Show resolved Hide resolved
@rbehrends
Copy link
Contributor Author

Seems sensible enough to me. Of course a a test, e.g. in tst/testlibgap, would be super nice... In the meantime, at least a comment somewhere stating that this is (or rather: will be) used by the GAP-Julia interface would be appropriate. Else this might get remove again for being unused...

This is easily doable, yes, and makes good sense.

@fingolfin
Copy link
Member

Unfortunately we cannot easily backport this to GAP 4.11, as GAP_TRY & GAP_CATCH are the result of a substantial kernel refactoring in the master branch.

@fingolfin fingolfin merged commit e6f3801 into gap-system:master Jul 23, 2020
@fingolfin fingolfin deleted the try-catch-handlers branch July 23, 2020 12:36
@PaulaHaehndel PaulaHaehndel self-assigned this Feb 16, 2021
@PaulaHaehndel PaulaHaehndel added the release notes: added PRs introducing changes that have since been mentioned in the release notes label Feb 16, 2021
@PaulaHaehndel PaulaHaehndel removed their assignment Feb 16, 2021
@PaulaHaehndel PaulaHaehndel changed the title Allow registering global handlers for GAP_TRY/GAP_CATCH. Allow registering global handlers for GAP_TRY/GAP_CATCH Feb 17, 2021
@olexandr-konovalov olexandr-konovalov mentioned this pull request Feb 19, 2021
Closed
@olexandr-konovalov olexandr-konovalov mentioned this pull request Mar 23, 2021
Merged
@fingolfin fingolfin added release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes and removed release notes: added PRs introducing changes that have since been mentioned in the release notes labels Aug 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChrisJefferson ChrisJefferson ChrisJefferson left review comments

@fingolfin fingolfin fingolfin approved these changes

Assignees
No one assigned
Labels
kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes topic: error handling topic: julia Julia GC integration and related matters topic: kernel
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@rbehrends @coveralls @fingolfin @ChrisJefferson @PaulaHaehndel